Isolate KVS-backed stores by store ID and scoped clearing - #7004
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: d2669c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — this PR is a focused regression test capturing the KVS-backed store isolation bug described in the audit. No new issues found in the test changes themselves.
Reviewed changes
Reviewed the addition of a single reproduction test in packages/effect/test/unstable/persistence/KeyValueStore.test.ts that asserts store IDs used with Persistence.layerBackingKvs properly isolate keys and scoped clear operations. The branch currently contains only the failing reproduction test set; the implementation fix is intentionally expected to follow on this same branch per the PR description.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
| @@ -88,6 +88,24 @@ export const testLayer = <E>(layer: Layer.Layer<KeyValueStore.KeyValueStore, E>) | |||
|
|
|||
| describe("KeyValueStore / layerMemory", () => testLayer(KeyValueStore.layerMemory)) | |||
|
|
|||
| describe("Persistence / layerBackingKvs", () => { | |||
There was a problem hiding this comment.
Can you move this test into the shared test suite.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
Reviewed the implementation fix that landed since the prior pullfrog review. The new commit changes layerBackingKvs to encode store IDs and entry keys as JSON composite keys, maintains a per-store index so clear can remove only that store's entries, serializes index/entry mutations with a semaphore, and adds a patch changeset.
- Implemented scoped key encoding and per-store index tracking in
layerBackingKvs. - Added a patch changeset describing the store-isolation and scoped-clear behavior change.
- Left the regression test in a standalone
describeblock (the existing human reviewer request to move it into the sharedtestLayersuite is still open).
⚠️ Multi-runtime index updates are not atomic
The per-store index key is read-modified-written under a single in-process semaphore. Because the KeyValueStore interface has no compare-and-swap or transaction primitive, concurrent writers in different runtimes that share the same backing store can lose index updates. Entries whose keys are dropped from the index become orphaned and will survive clear. Since the module is intended for use across workers/processes sharing a backing store, this limitation should be documented in the layer's JSDoc and possibly the changeset unless the design changes.
Technical details
# Multi-runtime index safety in layerBackingKvs
## Affected sites
- `packages/effect/src/unstable/persistence/Persistence.ts:1041-1069` — `readIndex`, `addToIndex`, and `removeFromIndex` perform read-modify-write on a single index key.
- `packages/effect/src/unstable/persistence/Persistence.ts:1071-1172` — `set`, `remove`, and `clear` use a process-local semaphore, which does not coordinate with other runtimes.
## Required outcome
- Authors must decide whether the new orphan-entry risk under multi-runtime concurrency is acceptable for the unstable module and document it if so.
- If not, the design should avoid a single mutable index key (e.g., by adding a key-scan primitive to `KeyValueStore` or using backend-specific scoping).@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
| > = Layer.effect(BackingPersistence)(Effect.gen(function*() { | ||
| const backing = yield* KeyValueStore.KeyValueStore | ||
| const clock = yield* Clock.Clock | ||
| const semaphore = yield* Semaphore.make(1) |
There was a problem hiding this comment.
This semaphore serializes writes across every store created by this layer. Consider using a per-store semaphore so mutations of distinct storeIds can proceed concurrently while still preventing lost index updates within a store.
| backing.get(indexKey), | ||
| (value) => { | ||
| if (value === undefined) return Effect.succeed<Array<string>>([]) | ||
| try { |
There was a problem hiding this comment.
Prefer Effect.try over try/catch for parse failures; this block is new code and the repo convention avoids try/catch.
| try { | |
| (value) => { | |
| if (value === undefined) return Effect.succeed<Array<string>>([]) | |
| return Effect.try({ | |
| try: () => JSON.parse(value), | |
| catch: (cause) => cause | |
| }).pipe( | |
| Effect.flatMap((parsed) => | |
| Array.isArray(parsed) && parsed.every((key): key is string => typeof key === "string") | |
| ? Effect.succeed(parsed) | |
| : Effect.fail(new Error("Invalid persistence store index")) | |
| ) | |
| ) | |
| } |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
Distinct KVS-backed stores can alias each other's entries, and clearing one store clears all stores sharing the backing key-value store.
Important
This PR includes the focused regression test and the implementation fix.
KVS-backed stores are not isolated by store ID
Module:
PersistenceAudit ID:
unstable-state-p-2Severity / confidence: high / high
What happens
Distinct KVS-backed stores can alias each other's entries, and clearing one store clears all stores sharing the backing key-value store.
Why it happens
Store IDs and entry keys are concatenated without an unambiguous boundary, so store a/key bc aliases store ab/key c. The prefixed view also inherits the backing store's unscoped clear operation.
Expected behavior
BackingPersistence.make(storeId) creates a store scoped to that ID, and clear removes only that store's entries.
Relevant implementation
These links and excerpts are pinned to audit base
c9b56ab507f224426ee8388dc450da447ec4715f.packages/effect/src/unstable/persistence/Persistence.ts:1036-1039packages/effect/src/unstable/persistence/Persistence.ts:1108-1114packages/effect/src/unstable/persistence/KeyValueStore.ts:300-309View problematic code at
packages/effect/src/unstable/persistence/Persistence.ts:1036-1039View exact lines on GitHub
View problematic code at
packages/effect/src/unstable/persistence/Persistence.ts:1108-1114View exact lines on GitHub
View problematic code at
packages/effect/src/unstable/persistence/KeyValueStore.ts:300-309View exact lines on GitHub
Reproduction
pnpm test --run packages/effect/test/unstable/persistence/KeyValueStore.test.tsObserved failure: Store a/key bc read the value written to store ab/key c; source inspection also confirmed global clear.
Implementation
KVS-backed stores now encode store IDs and entry keys as unambiguous composite keys. A per-store key index scopes clear operations without clearing unrelated entries in the shared backing key-value store.
Validated with:
Audit provenance
c9b56ab507f224426ee8388dc450da447ec4715fc9b56ab507f224426ee8388dc450da447ec4715funstable-state-p-2Closes EFF-441